[INSTR COMBINE] implement combine select into cast #114612
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-transforms Author: Cr0a3 (Cr0a3) ChangesHi, into this: Fixes #107034 @weliveindetail Full diff: https://github.com/llvm/llvm-project/pull/114612.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 999ad1adff20b8..2758de88c899f6 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3645,12 +3645,47 @@ static bool hasAffectedValue(Value *V, SmallPtrSetImpl<Value *> &Affected,
return false;
}
+// Checks for following pattern:
+// ```
+// %any1 = select i1 %any0, float 1.000000e+00, float 0.000000e+00
+// ```
+// which then gets folded into:
+// ```
+// %any1 = uitofp i1 %any0 to float
+// ```
+// (also works with double)
+static std::optional<Instruction*> mabyeFoldIntoCast(Value* CondVal, ConstantFP *TrueVal, ConstantFP *FalseVal, Type *SelType, llvm::StringRef out) {
+ if (TrueVal->getValueAPF().convertToDouble() != 1.0) {
+ return std::optional<Instruction*>();
+ }
+
+ if (FalseVal->getValueAPF().convertToDouble() != 0.0) {
+ return std::optional<Instruction*>();
+ }
+
+ return CastInst::Create(llvm::Instruction::UIToFP, CondVal, SelType, out);
+}
+
+
Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
Value *CondVal = SI.getCondition();
Value *TrueVal = SI.getTrueValue();
Value *FalseVal = SI.getFalseValue();
Type *SelType = SI.getType();
+
+ if (ConstantFP *True = dyn_cast<ConstantFP>(TrueVal)) {
+ if (ConstantFP *False = dyn_cast<ConstantFP>(FalseVal)) {
+ if (SelType->isFloatTy() || SelType->isDoubleTy()) {
+ std::optional<Instruction*> folded = mabyeFoldIntoCast(CondVal, True, False, SelType, SI.getName());
+
+ if (folded.has_value()) {
+ return folded.value();
+ }
+ }
+ }
+ }
+
if (Value *V = simplifySelectInst(CondVal, TrueVal, FalseVal,
SQ.getWithInstruction(&SI)))
return replaceInstUsesWith(SI, V);
diff --git a/llvm/test/Transforms/InstCombine/2024-11-07-FoldSelectIntoCast.ll b/llvm/test/Transforms/InstCombine/2024-11-07-FoldSelectIntoCast.ll
new file mode 100644
index 00000000000000..98af70d0188629
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/2024-11-07-FoldSelectIntoCast.ll
@@ -0,0 +1,8 @@
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+define noundef float @ifelse(i1 noundef zeroext %x) unnamed_addr {
+start:
+; CHECK: %.= uitofp i1 %x to float
+ %. = select i1 %x, float 1.000000e+00, float 0.000000e+00
+ ret float %.
+}
\ No newline at end of file
|
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
Please take a look at https://llvm.org/docs/InstCombineContributorGuide.html for guidelines for contributing to InstCombine, esp regarding proofs and tests.
But even before that, wasn't the consensus from #107034 that this is better handled in the backend on the SDAG level, rather than in InstCombine?
You can test this locally with the following command:git-clang-format --diff e549ec529c0c39cfa2fdf4ab919de406a0ef89cb ac1b070955a8de141043847b0813bf8a9e3918cd --extensions cpp -- llvm/lib/Transforms/InstCombine/InstCombineSelect.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 2758de88c8..4e86f30100 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3654,30 +3654,31 @@ static bool hasAffectedValue(Value *V, SmallPtrSetImpl<Value *> &Affected,
// %any1 = uitofp i1 %any0 to float
// ```
// (also works with double)
-static std::optional<Instruction*> mabyeFoldIntoCast(Value* CondVal, ConstantFP *TrueVal, ConstantFP *FalseVal, Type *SelType, llvm::StringRef out) {
+static std::optional<Instruction *>
+mabyeFoldIntoCast(Value *CondVal, ConstantFP *TrueVal, ConstantFP *FalseVal,
+ Type *SelType, llvm::StringRef out) {
if (TrueVal->getValueAPF().convertToDouble() != 1.0) {
- return std::optional<Instruction*>();
+ return std::optional<Instruction *>();
}
if (FalseVal->getValueAPF().convertToDouble() != 0.0) {
- return std::optional<Instruction*>();
+ return std::optional<Instruction *>();
}
return CastInst::Create(llvm::Instruction::UIToFP, CondVal, SelType, out);
}
-
Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
Value *CondVal = SI.getCondition();
Value *TrueVal = SI.getTrueValue();
Value *FalseVal = SI.getFalseValue();
Type *SelType = SI.getType();
-
if (ConstantFP *True = dyn_cast<ConstantFP>(TrueVal)) {
if (ConstantFP *False = dyn_cast<ConstantFP>(FalseVal)) {
if (SelType->isFloatTy() || SelType->isDoubleTy()) {
- std::optional<Instruction*> folded = mabyeFoldIntoCast(CondVal, True, False, SelType, SI.getName());
+ std::optional<Instruction *> folded =
+ mabyeFoldIntoCast(CondVal, True, False, SelType, SI.getName());
if (folded.has_value()) {
return folded.value();
|
That's right, they noted it in the ticket. Maybe the comments here can help finding a better place for it? https://github.com/llvm/llvm-project/pull/107732/files#r1749823010 FYI @Cr0a3 was asking for a good first issue and we spotted this one quickly. After all, it has that label. Not sure it's still the case if it must be implemented in ISel? Anyway, good first attempt :) |
Yeah, looking at the discussion in that PR, this issue may not be as simple as it appeared on the surface :) If you're looking for a simple middle-end issue, I just opened #114820, though maybe it's not a particularly exciting one... |
|
Thank you very much |
4 participants
Hi,
I implemented the instruction combination of this:
into this:
Fixes #107034
@weliveindetail